module_adapter: generic: Fix use after free #10297

dbaluta · 2025-10-13T07:38:58Z

Remove any containers from the free container list so that we don't keep pointers to containers that are no longer used and will be freed when container chunks are released below.

Leaving those nodes in the free container list would cause use-after-free on subsequent allocations.

While at it, make sure all resource lists are reset.

Copilot

Pull Request Overview

This PR fixes a use-after-free vulnerability in the module adapter's generic resource management by properly cleaning up free container lists and resetting resource accounting.

Remove containers from the free container list before freeing container chunks to prevent dangling pointers
Reset all resource lists and heap usage accounting to ensure clean state after freeing resources

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tmleman

LGTM. Fails in CI looks like env issue, lets wait for re-run.

lyakh

Do I understand it correctly, that this is addressing a case, when "chunks" of containers are freed, then they happen to be allocated again and the new owner finds old data in them? If that my understanding is correct, then I don't think this is a right approach. If we want to do this, then we'd have to memset(0) all memory buffers before freeing.

dbaluta · 2025-10-16T08:56:33Z

@lyakh No.

Container chunks have allocated up to 16 containers. We store references to these containers in the free containers list.

Then when we free a chunk, we free the memory for this 16 containers but we do not remove the references from the containers free list. This means that the containers list will contain some pointers to freed memory.

Next time we allocate a container chunk we allocate again new 16 containers chunks and we place references to this new containers in the free containers list. But this list already has references to the older containers (because we never removed them from free container list).

What happens is that at some point one of the older references is picked to be used and we access freed memory.

using memset won't help.

kv2019i

Excellent catch @dbaluta ! @jsarha is not today available to comment, but I think the original implementation did not expect the module to be used again after mod_free_all(). In module_adapter_free(), there's a call to mod_free_all() but afterwards the module resources are also freed. But I now see in cadence.c, mod_free_all() is called in multiple places, including cadence_codec_reset(). And yeah, mod_free_all() does not leave the containers in proper state.

kv2019i · 2025-10-16T12:17:04Z

src/audio/module_adapter/module/generic.c

+	list_init(&res->res_list);
+	list_init(&res->free_cont_list);
+	list_init(&res->cont_chunk_list);
+	res->heap_usage = 0;


Hmm, maybe call to "mod_resource_init()" here?

sure, will send a new version asap.

kv2019i · 2025-10-16T12:21:54Z

@dbaluta @lgirdwood I think we need this fix in stable-v2.14 as well.

dbaluta · 2025-10-16T14:57:05Z

Changes since v1:

added mod_resource_init as per @kv2019i feedback.

I will backport it to v2.14 once this is merged.

lyakh · 2025-10-17T06:28:51Z

What happens is that at some point one of the older references is picked to be used and we access freed memory.

@dbaluta right, yes, thanks for explaining. But usually mod_free_all() is only called when the module is freed, so no new allocations should be performed there? I see one "abuse" though - in cadence.c, where mod_free_all() is called durinig a reset... Is that what you're referring to? Then it might be a problem. Not sure about the current state, but in the future I'd expect also the infrastructure to allocate per-module memory using the mod_alloc*() API... But maybe I'm wrong. I'll remove my "request for change" but there's more to clarify here... Thanks!

(partially) clarified

Factor out resource initialization so that it can be reused. While at it get rid of md variable. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Remove any containers from the free container list so that we don't keep pointers to containers that are no longer used and will be freed when container chunks are released below. Leaving those nodes in the free container list would cause use-after-free on subsequent allocations. While at it, make sure all resource lists are reset. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

jsarha

Some room for optimization, but looks good otherwise.

jsarha · 2025-10-17T08:26:58Z

src/audio/module_adapter/module/generic.c

+		struct module_resource *container =
+			container_of(list, struct module_resource, list);
+
+		list_item_del(&container->list);


There should be no need to do this anymore, after the mod_resource_init(mod) call was added. It will reinitialize &res->free_cont_list at the end of the function. No need to remove the containers one by one.

jsarha · 2025-10-17T09:19:53Z

src/audio/module_adapter/module/generic.c

 			container_of(list, struct module_resource, list);

 		free_contents(mod, container);
 		list_item_del(&container->list);


Actually also this list_item_del()-call could be removed now that &res->res_list is also reinitialized at the end of the function when mod_resource_init() is called.

kv2019i · 2025-10-17T15:39:49Z

@dbaluta Feel free to either address Jyri's suggestions, or to just go ahead and merge this. If needed @jsarha can do a follow-up PR to optimize the the free_all implementation.

lgirdwood

Ack - this will be needed for v2.14

dbaluta · 2025-10-19T18:02:06Z

@kv2019i merged this as it is, as it is following a logical cleanup of the lists, in reverse wrt the init part.

@jsarha please send a follow up with the simplification if you think it is necessary.

jsarha · 2025-10-20T22:05:43Z

@kv2019i merged this as it is, as it is following a logical cleanup of the lists, in reverse wrt the init part.

@jsarha please send a follow up with the simplification if you think it is necessary.

@dbaluta the optimize PR is here: #10316

Copilot AI review requested due to automatic review settings October 13, 2025 07:38

dbaluta requested review from kv2019i, lbetlej, lgirdwood, mmaka1 and plbossart as code owners October 13, 2025 07:38

Copilot AI reviewed Oct 13, 2025

View reviewed changes

dbaluta force-pushed the fix_use_after_free branch 2 times, most recently from 53da97f to 01e4e87 Compare October 13, 2025 07:41

dbaluta requested review from LaurentiuM1234 and iuliana-prodan October 13, 2025 08:30

LaurentiuM1234 approved these changes Oct 13, 2025

View reviewed changes

tmleman reviewed Oct 13, 2025

View reviewed changes

lgirdwood approved these changes Oct 13, 2025

View reviewed changes

iuliana-prodan approved these changes Oct 13, 2025

View reviewed changes

lyakh previously requested changes Oct 15, 2025

View reviewed changes

kv2019i requested a review from jsarha October 16, 2025 12:11

kv2019i approved these changes Oct 16, 2025

View reviewed changes

dbaluta force-pushed the fix_use_after_free branch from 01e4e87 to 68b7672 Compare October 16, 2025 14:48

dbaluta force-pushed the fix_use_after_free branch from 68b7672 to fb17ec9 Compare October 17, 2025 06:28

dbaluta added 2 commits October 17, 2025 09:29

module_adapter: generic: Introduce mod_generic_init

c952d07

Factor out resource initialization so that it can be reused. While at it get rid of md variable. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

jsarha approved these changes Oct 17, 2025

View reviewed changes

jsarha reviewed Oct 17, 2025

View reviewed changes

lgirdwood reviewed Oct 17, 2025

View reviewed changes

dbaluta merged commit 259c32b into thesofproject:main Oct 19, 2025
39 of 45 checks passed

module_adapter: generic: Fix use after free #10297

module_adapter: generic: Fix use after free #10297

Uh oh!

Conversation

dbaluta commented Oct 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

tmleman left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

dbaluta Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Oct 16, 2025

Uh oh!

dbaluta commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Oct 17, 2025

Uh oh!

jsarha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsarha Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsarha Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Oct 17, 2025

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbaluta commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsarha commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dbaluta commented Oct 16, 2025 •

edited

Loading

dbaluta commented Oct 16, 2025 •

edited

Loading

jsarha left a comment •

edited

Loading

jsarha Oct 17, 2025 •

edited

Loading

dbaluta commented Oct 19, 2025 •

edited

Loading